fix: cap unbounded collections and convert panicking index access to … - #268

Summary

• Capped every unbounded, caller-supplied collection in the public API behind a typed TooManyItems error, following the existing MAX_ITEMS_PER_ESCROW precedent:
  • get_escrows(limit) — new MAX_PAGE_SIZE (50); over-limit values are silently clamped rather than erroring, since pagination is a read-only view.
  • batch_collect_fees(escrow_ids) — already capped by MAX_ESCROWS_PER_BATCH from [high] batch_collect_fees reports collected fees but transfers no funds #259; verified still enforced.
  • create_bulk_escrows(requests) — new MAX_BULK_ESCROWS_PER_CALL (20).
  • create_milestone_escrow(milestones) — new MAX_MILESTONES_PER_ESCROW (50).
  • create_group_buy_escrow(buyers) — new MAX_GROUP_BUY_BUYERS (50).
• MAX_PAGE_SIZE and MAX_BULK_ESCROWS_PER_CALL aren't arbitrary — they're sized to actually fit inside Soroban's per-transaction resource budget (~100 total footprint entries, ~50 write entries). A first pass at 100/50 looked "capped" but still blew the real ledger footprint/write budget in testing (a full-size call would fail on a real network even after the cap), so both were brought down with headroom.
• Converted every caller-reachable panicking index lookup (.get(i).unwrap()) into a typed error instead:
  • release_item → ItemNotFound (collapsed a separate bounds check + unwrap() into one .get(...).ok_or(...)?)
  • complete_milestone → MilestoneNotFound (same pattern)
  • fund_group_buy / withdraw_group_buy_contribution → Unauthorized on their internal buyer-contribution lookup (defensive — the index is derived from enumerate() over the same vec so it's always in bounds, but this removes the panic path entirely)
• Audited every remaining unwrap()/expect() in lib.rs per the issue's checklist:
  • process_seller_transfer and execute_mediation_settlement's fee-collector lookups now return ContractError::InvalidFeeConfig instead of panicking if the contract was never initialized (process_seller_transfer's signature changed to Result<i128, ContractError>; all 5 call sites updated to propagate with ?)
  • check_metadata_access's admin lookup no longer panics on an uninitialized contract
  • accept_admin's admin lookup returns NotAdmin instead of panicking
  • create_milestone_escrow / create_group_buy_escrow's post-creation escrow re-fetch returns EscrowNotFound instead of panicking (defensive — the record was just written by create_escrow_internal)
  • The one remaining expect() (add_i128's counter-overflow guard) is left as-is with a comment explaining why it's unreachable in practice: it's a global i128 analytics counter incremented by amounts bounded by real token supply, many orders of magnitude below i128::MAX.
• Added 5 tests: an out-of-range complete_milestone index, plus at-the-limit / over-the-limit coverage for each of the four new caps (milestones, bulk escrows, group-buy buyers, get_escrows page size).

Scope note

The issue named batch_collect_fees and get_escrows explicitly. While implementing the fix I found the same unbounded-collection bug in three more entrypoints (create_bulk_escrows, create_milestone_escrow, create_group_buy_escrow) and fixed those too, since they're the identical bug class the issue is about ("harden all caller-supplied input"). Flagging this in case reviewers want it split out.

Linked Issue

Closes #260

CI Checklist

• cargo fmt --all -- --check
• cargo clippy --all-targets -- -D warnings
• cargo test — 128 unit tests + 2 integration tests pass (was 123 unit tests before this PR)
• ./scripts/build_wasm.sh

Notes for Reviewers

• process_seller_transfer changing from -> i128 to -> Result<i128, ContractError> is the widest-blast-radius change here — it's called from release_escrow, release_item, claim_disputed_funds, complete_milestone, and trigger_time_lock_release. All five call sites just gained a ?; no behavior changes on the success path.
• The fund_group_buy / withdraw_group_buy_contribution index conversions are defensive rather than fixing a reachable bug — there's no public index parameter on those functions, the index is found internally via enumerate() over the buyer list. Included since the issue's technical context explicitly calls out those two line locations.
• Error code numbering is unchanged — only new error usages (InvalidFeeConfig, TooManyItems, EscrowNotFound, Unauthorized, NotAdmin) at existing variants, no new variants added.